Fix for plus signs in data and verified array #119
Conversation
$_POST can decode plus signs incorrectly and should not be used. I have done extensive testing so far and have not found a better way than this to fix this problem from this end. This problem will only be completely solved when the posted data is sent with plus signs always encoded to %2B.
BigRedBot
changed the title
BigRedBot
changed the title
php/PaypalIPN.php
Outdated
| @@ -63,7 +63,7 @@ public function getPaypalUri() | |||
| * Verification Function | |||
| * Sends the incoming post data back to PayPal using the cURL library. | |||
| * | |||
| * @return bool | |||
| * @return array or false | |||
There was a problem hiding this comment.
The docblock should be array|bool
php/PaypalIPN.php
Outdated
| @@ -75,31 +75,31 @@ public function verifyIPN() | |||
| $raw_post_data = file_get_contents('php://input'); | |||
| $raw_post_array = explode('&', $raw_post_data); | |||
| $myPost = array(); | |||
| $magic_quotes_enabled = false; | |||
There was a problem hiding this comment.
Magic quotes has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 5.4.0.
I don't see the value in adding a check here - I would rather specify a minimum PHP version,
http://php.net/manual/en/security.magicquotes.php
There was a problem hiding this comment.
I agree with this.
php/PaypalIPN.php
Outdated
| // Convert plus signs to spaces | ||
| $myPost[$keyval[0]] = urldecode($keyval[1]); | ||
| if ($magic_quotes_enabled) { | ||
| $myPost[$keyval[0]] = stripslashes($myPost[$keyval[0]]); |
There was a problem hiding this comment.
In my option the logic above has to much nesting, and it's hard to understand what is going on.
I'm not sure why this is required, I have been running the current version of this script in production for > 1 year without any issues.
Can you update the PR with some more information about the issue, and some replication steps?
| return true; | ||
| } else { | ||
| return false; | ||
| return $myPost; |
There was a problem hiding this comment.
I think this class should have a single responsibility, to verify that the IPN data is valid.
You can then retrieve the post data from $__POST or your frameworks request object. I do not see a reason why this needs to return data.
There was a problem hiding this comment.
At least from the sandbox, it is in some cases receiving incorrectly encoded data. This can cause the data in $_POST to be incorrect. Since we can compensate for this and rebuild the data, then we should at that point use the validated rebuilt data.
There is pretty much no reason for it not to include the verified array. It already has it, so why not just return it?
There was a problem hiding this comment.
The only other option (that has not been done yet), is to have paypal's servers always make sure that $_POST is always correct no matter where the data is coming from. This would still require the use of the rawurlencode function, to be sure that plus signs are correctly verified.
php/PaypalIPN.php
Outdated
| } | ||
| return false; |
There was a problem hiding this comment.
It's best not to return multiple types from a function - it would make more sense to return null vs false here.
php/PaypalIPN.php
Outdated
| if (substr_count($keyval[1], '+') === 1) { | ||
| $keyval[1] = str_replace('+', '%2B', $keyval[1]); | ||
| // Since we do not want the plus signs in the date and email strings to be encoded to a space, we use rawurldecode instead of urldecode | ||
| if (($keyval[0] === 'payment_date' && substr_count($keyval[1], '+') === 1) || strpos(rawurldecode($keyval[1]), ' ') !== false || ($_POST["test_ipn"] == 1 && filter_var($keyval[1], FILTER_VALIDATE_EMAIL))) { |
There was a problem hiding this comment.
This line is also much too long.
|
Using the function urlencode with an email address that contains plus signs, may cause the plus signs to encode as a space instead of a plus. This would prevent the data from being validated with email addresses with plus signs. It is safer to use rawurlencode instead. |
$_POST can in some instances decode plus signs incorrectly, and can not in all possible instances be trusted to have decoded the data correctly.
With this change, you may instead use an array that has had its contents completely verified for accuracy.
Using the function urlencode with an email address that contains plus signs, may cause the plus signs to encode as a space instead of a plus. This would prevent the data from being validated with email addresses with plus signs. It is safer to use rawurlencode instead.